-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PEP 695] Partial support for new type parameter syntax in Python 3.12 #17233
Conversation
Wonderful, thanks! I'll do a review within the next week. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Thanks for working on it @JukkaL 🚀
Left some comments on stuff I noticed. I've also been able to start testing this change with real code today. Will post a separate comment about it soon.
mypy/fastparse.py
Outdated
if sys.version_info >= (3, 12): | ||
ast_TypeAlias = ast3.TypeAlias | ||
ParamSpec = ast3.ParamSpec | ||
TypeVarTuple = ast3.TypeVarTuple | ||
else: | ||
ast_TypeAlias = Any | ||
ParamSpec = Any | ||
TypeVarTuple = Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use ast_ParamSpec
and ast_TypeVarTuple
here instead, just to prevent accidental name conflicts in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
mypy/semanal.py
Outdated
for p in s.type_args: | ||
upper_bound = p.upper_bound or self.object_type() | ||
fullname = self.qualified_name(p.name) | ||
type_params.append( | ||
( | ||
p.name, | ||
TypeVarExpr( | ||
p.name, | ||
fullname, | ||
[], | ||
upper_bound, | ||
default=AnyType(TypeOfAny.from_omitted_generics), | ||
), | ||
) | ||
) | ||
all_type_params_names.append(p.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need to differentiate between all three TypeVar kinds as well? ParamSpec and TypeVarTuple are supported for type
statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for p.kind
and adding ParamSpecExpr
seem to fix Issue 1 + 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed now.
test-data/unit/check-python312.test
Outdated
class C[T]: | ||
def m[S](self, x: S) -> T: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't x
be considered Any
here as S
is only used once? Maybe something like this?
class C[T]: | |
def m[S](self, x: S) -> T: ... | |
class C[T]: | |
def m[S](self, x: S) -> T | S: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good idea, done.
[case testPEP695ParamSpec] | ||
# flags: --enable-incomplete-feature=NewGenericSyntax | ||
from typing import Callable | ||
|
||
def g[**P](f: Callable[P, None], *args: P.args, **kwargs: P.kwargs) -> None: | ||
f(*args, **kwargs) | ||
f(1, *args, **kwargs) # E: Argument 1 has incompatible type "int"; expected "P.args" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to test ParamSpec
with the type
statement here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test case.
[case testPEP695TypeVarTuple] | ||
# flags: --enable-incomplete-feature=NewGenericSyntax | ||
|
||
def f[*Ts](t: tuple[*Ts]) -> tuple[*Ts]: | ||
reveal_type(t) # N: Revealed type is "Tuple[Unpack[Ts`-1]]" | ||
return t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to test TypeVarTuple
with the type
statement here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test case.
As mentioned before, I've started testing this change with Home Assistant. After I saw the PR today, I begun looking through the codebase and converting TypeVar / ParamSpec / TypeVarTuple calls as well as some type aliases to the PEP 695 syntax. Some general observations The changes here works fairly well. I only encountered some smaller issues. You mentioned in the description that the work here is incomplete, so you might already plan to address some of them. Issue 1 - # mypy: enable-incomplete-feature=NewGenericSyntax
from typing import Callable
type _FuncType[**_P] = Callable[_P, None] # Error
Edit: This seems to be fixed by checking for Issue 2 - Decorator typing with # mypy: enable-incomplete-feature=NewGenericSyntax
from typing import Callable, Concatenate
class Device: ...
type _FuncType[_T, **_P] = Callable[Concatenate[_T, _P], None] # Error
def decorator[_DeviceT: Device, **_P](func: _FuncType[_DeviceT, _P]) -> _FuncType[_DeviceT, _P]:
def wrapped(self: _DeviceT, *args: _P.args, **kwargs: _P.kwargs) -> None:
...
return wrapped File "/.../mypy/typeanal.py", line 344, in visit_unbound_type_nonoptional
assert isinstance(tvar_def, ParamSpecType)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Edit: This seems to be fixed by checking for Issue 3 - maximum semantic analysis iteration count reached # mypy: enable-incomplete-feature=NewGenericSyntax
from typing import cast
class A:
def func[T: int](self, value: T) -> T:
return cast(T, value)
The same issue can be seen here: # mypy: enable-incomplete-feature=NewGenericSyntax
def func[T](val: T) -> T:
return unknown() # Error Issue 4 - Type alias with forward reference as upper_bound # mypy: enable-incomplete-feature=NewGenericSyntax
type _FuncType[_T: Device] = _T
class Device: ...
|
mypy/semanal.py
Outdated
) | ||
all_type_params_names.append(p.name) | ||
|
||
self.push_type_args(s.type_args, s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe defer if push_type_args
returned False
e.g. when a forward reference is used as upper_bound? This would fix Issue 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this works. Fixed the issue.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@cdce8p I think I've fixed all the issues you reported, except for issue 3, which needs support for the new PEP 695 scoping mechanism. |
This comment has been minimized.
This comment has been minimized.
And I'm thinking of fixing scoping in a follow-up PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JukkaL 👍🏻 The latest commits indeed fixed all issues. Except for no. 3 but as you mentioned that still needs support for scoping. Makes sense to do that separately. At least with 196b2a3 it doesn't crash anymore.
One area I still get a few errors is with generic classes, overloads and TypeVarTuples. Haven't had time to take a closer look so far, unfortunately.
However, as it's easy enough to work around, just continue using Generic
for it, I don't think this should block the PR. If it persists and when I've time to narrow it down, I can always open a new issue.
--
Thanks again for working on this. I look forward to using the new syntax 🚀
Feel free to ping me if I should check out other PRs and test them. Happy to do so.
Hmm, maybe the overload error is actually related to variance. In the example here from typing import Callable, Generic, TypeVar
class Job[_R_co]:
def __init__(self, target: Callable[[], _R_co]) -> None:
self.target = target
def func(
action: Job[int | None],
a1: Job[int | None],
a2: Job[int],
a3: Job[None],
) -> None:
action = a1
action = a2 # error
action = a3 # error error: Incompatible types in assignment (expression has type "Job[int]", variable has type "Job[int | None]") [assignment]
error: Incompatible types in assignment (expression has type "Job[None]", variable has type "Job[int | None]") [assignment] It works fine when using _S_co = TypeVar("_S_co", covariant=True)
class Job2(Generic[_S_co]):
def __init__(self, target: Callable[[], _R_co]) -> None:
self.target = target
def func2(
action: Job2[int | None],
a1: Job2[int],
a2: Job2[int | None],
a3: Job2[None],
) -> None:
action = a1
action = a2
action = a3 |
After a discussion in a pyright issue, I seems from typing import Callable, Generic, TypeVar, Final
class Job[_R_co]:
def __init__(self, target: Callable[[], _R_co]) -> None:
self.target: Final = target # <-- marked as Final
def func(
action: Job[int | None],
a1: Job[int | None],
a2: Job[int],
a3: Job[None],
) -> None:
action = a1
action = a2 # error
action = a3 # error Current mypy output: error: Incompatible types in assignment (expression has type "Job[int]", variable has type "Job[int | None]") [assignment]
error: Incompatible types in assignment (expression has type "Job[None]", variable has type "Job[int | None]") [assignment] |
@cdce8p Variance inference should be fixed now. It was broken when using real typeshed stubs. And thanks for the testing and the detailed bug reports -- they've been really helpful! |
This comment has been minimized.
This comment has been minimized.
Also avoid inferring variances repeatedly.
Thanks! Can confirm the last commit also fixed the overload issues I saw. AFAICT this PR looks good now 🚀 |
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
I'll wait one more day before merging, in case somebody else wants to review this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great.
If I'm not mistaken there's some missing serialization code though.
for p in type_params: | ||
bound = None | ||
values: list[Type] = [] | ||
if isinstance(p, ast_ParamSpec): # type: ignore[misc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can wait for another PR, but we should also check for the .default_value
here for PEP 696 support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably do that in a followup. Should be easy enough to implement support for PEP 696 here as well, although that requires Python 3.13 anyway.
) -> None: | ||
super().__init__() | ||
self.arguments = arguments or [] | ||
self.arg_names = [None if arg.pos_only else arg.variable.name for arg in self.arguments] | ||
self.arg_kinds: list[ArgKind] = [arg.kind for arg in self.arguments] | ||
self.max_pos: int = self.arg_kinds.count(ARG_POS) + self.arg_kinds.count(ARG_OPT) | ||
self.type_args: list[TypeParam] | None = type_args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new field should be serialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we actually need that here. Could be missing something but my understanding is that type_args
is only used for the SemanticAnalyzer
so there isn't a need to serialize it.
If it were necessary, mypy would've probably crashed for me while testing the PR with Home Assistant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be right, I don't actually understand how our serialization works :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is just part of the syntax and doesn't get used beyond semantic analysis.
Just released https://pypi.org/project/mypy-dev/1.11.0a2/ |
Add basic support for most features of PEP 695. It's still not generally useful, but it
should be enough for experimentation and testing. I will continue working on
follow-up PRs after this has been merged.
This is currently behind a feature flag:
--enable-incomplete-feature=NewGenericSyntax
These features, among other things, are unimplemented (or at least untested):
The trickiest remaining thing is probably variance inference in cases where some types
aren't ready (i.e. not inferred) when we need variance. I have some ideas about how to
tackle this, but it might need significant work. Currently the idea is to infer variance
on demand when we need it, but we may need to defer if variance can't be calculated,
for example if a type of an attribute is not yet ready. The current approach is to fall
back to covariance in some cases, which is not ideal.
Work on #15238.